Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

return histograms for time metrics #1005

Merged
merged 2 commits into from
Jul 11, 2024
Merged

return histograms for time metrics #1005

merged 2 commits into from
Jul 11, 2024

Conversation

Keksoj
Copy link
Member

@Keksoj Keksoj commented Oct 18, 2023

Up until now, time metrics were returned by Sōzu, and rendered in the CLI, as percentiles:

┌────────────────────────┬─────────┬──────┬──────┬──────┬───────┬────────┬─────────┬──────┐
│ Percentiles            │ samples │ p50  │ p90  │ p99  │ p99.9 │ p99.99 │ p99.999 │ p100 │
├────────────────────────┼─────────┼──────┼──────┼──────┼───────┼────────┼─────────┼──────┤
│ accept_queue.wait_time │ 6       │ 0    │ 0    │ 0    │ 0     │ 0      │ 0       │ 0    │
├────────────────────────┼─────────┼──────┼──────┼──────┼───────┼────────┼─────────┼──────┤
│ epoll_time             │ 40      │ 1000 │ 1001 │ 1001 │ 1001  │ 1001   │ 1001    │ 1001 │
├────────────────────────┼─────────┼──────┼──────┼──────┼───────┼────────┼─────────┼──────┤
│ event_loop_time        │ 40      │ 0    │ 0    │ 4    │ 4     │ 4      │ 4       │ 4    │
├────────────────────────┼─────────┼──────┼──────┼──────┼───────┼────────┼─────────┼──────┤
│ response_time          │ 6       │ 0    │ 0    │ 0    │ 0     │ 0      │ 0       │ 0    │
├────────────────────────┼─────────┼──────┼──────┼──────┼───────┼────────┼─────────┼──────┤
│ service_time           │ 6       │ 0    │ 0    │ 0    │ 0     │ 0      │ 0       │ 0    │
└────────────────────────┴─────────┴──────┴──────┴──────┴───────┴────────┴─────────┴──────┘

These percentiles are not convertible for a prometheus layer, however.

Since the time metrics are collected by sozu_lib's LocalDrain as hdrhistograms, why not return them in a histogram form as well? This would allow the Sozu prometheus connector to convert them to prometheus metrics.

This PR creates the protobuf type FilteredHistogram. Now, for each time metric:

  1. a Percentiles metric is produced, for instance response_time
  2. a FilteredHistogram is produced, named response_time_hist

@Keksoj Keksoj added this to the v0.16.0 milestone Oct 18, 2023
@Keksoj Keksoj self-assigned this Oct 18, 2023
@Keksoj
Copy link
Member Author

Keksoj commented Oct 26, 2023

Check issue #751 for inspiration

@Keksoj
Copy link
Member Author

Keksoj commented Nov 16, 2023

Actually I need to rebase the branch of PR #1004 into this one. #1004 refactors the display of metrics in a much cleaner way, and allows a way better display of JSON. This is crucial for development purposes.

@Keksoj Keksoj changed the base branch from main to fix-display-in-json-form November 16, 2023 21:35
@Keksoj Keksoj force-pushed the produce-histogram-metrics branch 3 times, most recently from f9a9218 to aa9df5f Compare November 20, 2023 12:05
@Keksoj Keksoj mentioned this pull request Nov 20, 2023
Merged
@Keksoj Keksoj force-pushed the fix-display-in-json-form branch 2 times, most recently from f0ba2cd to 88fb8cb Compare November 27, 2023 14:19
@Keksoj Keksoj changed the base branch from fix-display-in-json-form to main January 24, 2024 09:38
@Keksoj Keksoj force-pushed the produce-histogram-metrics branch 3 times, most recently from 8866640 to 0f9f6d7 Compare January 24, 2024 13:39
@Keksoj Keksoj changed the title return histograms for time metrics draft: return histograms for time metrics Feb 19, 2024
@Keksoj Keksoj force-pushed the produce-histogram-metrics branch 3 times, most recently from 3b5d7f7 to 7ed28fe Compare July 3, 2024 13:02
@Keksoj Keksoj force-pushed the produce-histogram-metrics branch from 7ed28fe to c0ab1fb Compare July 8, 2024 09:16
@Keksoj Keksoj changed the title draft: return histograms for time metrics return histograms for time metrics Jul 8, 2024
@Keksoj
Copy link
Member Author

Keksoj commented Jul 8, 2024

This is ready for review!

llenotre
llenotre reviewed Jul 8, 2024
View reviewed changes
command/src/proto/display.rs Show resolved Hide resolved
lib/src/metrics/local_drain.rs Outdated Show resolved Hide resolved
lib/src/metrics/local_drain.rs Outdated Show resolved Hide resolved
lib/src/metrics/local_drain.rs Outdated Show resolved Hide resolved
lib/src/metrics/local_drain.rs Outdated Show resolved Hide resolved
lib/src/metrics/local_drain.rs Outdated Show resolved Hide resolved
lib/src/metrics/local_drain.rs Outdated Show resolved Hide resolved
lib/src/metrics/local_drain.rs Outdated Show resolved Hide resolved
lib/src/metrics/local_drain.rs Show resolved Hide resolved
@Keksoj Keksoj force-pushed the produce-histogram-metrics branch 6 times, most recently from 8b43018 to 4894e69 Compare July 11, 2024 08:03
@Keksoj
metrics: return histograms for time metrics
64b558e 
create local type MetricsMap
implement receive_metric function for local types
add error management
iterate logarithmically (base 2) to produce prometheus buckets

optimize the listing of available metrics, with iterators
optimize function dump_cluster_metrics
@Keksoj
fix AggregatedMetrics::merge_metrics
15ac8eb 
the method would insert non-mergeable backend metrics.
@Keksoj Keksoj force-pushed the produce-histogram-metrics branch from 4894e69 to 15ac8eb Compare July 11, 2024 14:26
@Keksoj Keksoj merged commit b2965c0 into main Jul 11, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llenotre llenotre llenotre approved these changes

@FlorentinDUBOIS FlorentinDUBOIS Awaiting requested review from FlorentinDUBOIS FlorentinDUBOIS is a code owner

@Wonshtrum Wonshtrum Awaiting requested review from Wonshtrum Wonshtrum is a code owner

Assignees

@Keksoj Keksoj

Labels
enhancement metrics protobuf
Projects
Roadmap
Status: Done
Milestone
v0.16.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Keksoj @llenotre